refactor #112: reorganize config CLI and fix help-mode crash#230
Open
sushant-suse wants to merge 2 commits intoopenSUSE:mainfrom
Open
refactor #112: reorganize config CLI and fix help-mode crash#230sushant-suse wants to merge 2 commits intoopenSUSE:mainfrom
sushant-suse wants to merge 2 commits intoopenSUSE:mainfrom
Conversation
Signed-off-by: sushant-suse <[email protected]>
Coverage ReportFor commit 0779309 Click to expand Coverage Report Name Stmts Miss Branch BrPart Cover
--------------------------------------------------------------------------------
+ src/docbuild/models/deliverable.py 180 1 22 0 99.5%
+ src/docbuild/cli/cmd_check/process.py 58 0 22 1 98.8%
+ src/docbuild/models/manifest.py 111 1 12 1 98.4%
+ src/docbuild/utils/pidlock.py 79 1 14 1 97.8%
+ src/docbuild/cli/cmd_cli.py 109 1 16 3 96.8%
+ src/docbuild/cli/cmd_validate/process.py 178 5 52 4 96.1%
+ src/docbuild/cli/callback.py 35 0 10 2 95.6%
+ src/docbuild/utils/concurrency.py 69 3 18 1 95.4%
- src/docbuild/config/xml/stitch.py 47 5 12 0 88.1%
- src/docbuild/cli/cmd_metadata/metaprocess.py 215 26 66 13 82.6%
- src/docbuild/cli/cmd_config/list.py 43 9 20 3 71.4%
- src/docbuild/cli/cmd_config/validate.py 28 6 18 5 67.4%
- src/docbuild/cli/cmd_check/__init__.py 18 5 2 0 65.0%
- src/docbuild/cli/cmd_build/__init__.py 13 5 0 0 61.5%
- src/docbuild/cli/cmd_metadata/__init__.py 27 10 2 0 58.6%
--------------------------------------------------------------------------------
+ TOTAL 3062 78 746 34 96.4%
46 files skipped due to complete coverage. |
Signed-off-by: sushant-suse <[email protected]>
tomschr
requested changes
Apr 23, 2026
Contributor
tomschr
left a comment
tomschr
left a comment
There was a problem hiding this comment.
Thanks for all your efforts! 👍
I realized that I didn't tell you an important aspect. I've described it below.
Have added some suggestions around some design questions.
| items.append((new_key, v)) | ||
| return dict(items) | ||
|
|
||
| def _print_section(title: str, data: dict[str, Any], prefix: str, flat: bool, color: str) -> None: |
Contributor
There was a problem hiding this comment.
I would remove the underscore here as it's not really a very secret function. I would also introduce an additional line break as it's visually more readable (I think it should pass the ruff check).
Suggested change
| def _print_section(title: str, data: dict[str, Any], prefix: str, flat: bool, color: str) -> None: | |
| def print_section(title: str, data: dict[str, Any], prefix: str, flat: bool, color: str) -> None: |
| console.print(f"\n# {title}", style="blue") | ||
| print_json(data=data) | ||
|
|
||
| def _print_portal(doctypes: list[Any], flat: bool) -> None: |
Contributor
There was a problem hiding this comment.
Same here:
Suggested change
| def _print_portal(doctypes: list[Any], flat: bool) -> None: | |
| def print_portal(doctypes: list[Any], flat: bool) -> None: |
| print_json(data=data) | ||
|
|
||
| def _print_portal(doctypes: list[Any], flat: bool) -> None: | ||
| """Print the Portal and Doctype metadata section.""" |
Contributor
There was a problem hiding this comment.
Could you document the arguments in the docstring?
Comment on lines
+56
to
+62
| _print_section("Application Configuration", context.appconfig.model_dump(), "app", flat, "cyan") | ||
|
|
||
| if (env or show_all) and context.envconfig: | ||
| _print_section("Environment Configuration", context.envconfig.model_dump(), "env", flat, "yellow") | ||
|
|
||
| if (portal or show_all) and context.doctypes: | ||
| _print_portal(context.doctypes, flat) |
Contributor
There was a problem hiding this comment.
Adjust the function names accordingly:
Suggested change
| _print_section("Application Configuration", context.appconfig.model_dump(), "app", flat, "cyan") | |
| if (env or show_all) and context.envconfig: | |
| _print_section("Environment Configuration", context.envconfig.model_dump(), "env", flat, "yellow") | |
| if (portal or show_all) and context.doctypes: | |
| _print_portal(context.doctypes, flat) | |
| print_section("Application Configuration", context.appconfig.model_dump(), "app", flat, "cyan") | |
| if (env or show_all) and context.envconfig: | |
| print_section("Environment Configuration", context.envconfig.model_dump(), "env", flat, "yellow") | |
| if (portal or show_all) and context.doctypes: | |
| print_portal(context.doctypes, flat) |
Comment on lines
+42
to
+46
| if context.doctypes: | ||
| console.print(f"\n✅ [bold]Portals/Doctypes:[/bold] {len(context.doctypes)} discovered") | ||
| for doctype in context.doctypes: | ||
| name = getattr(doctype, "name", "Unknown") | ||
| console.print(f" [dim]- {name}[/dim]") |
Contributor
There was a problem hiding this comment.
That's not validation. Validation is much more.😉
After looking at your code, I remembered that I had done something similar for the Docserv/Portal validation. I introduced a docbuild validate subcommand back then.
See src/docbuild/cli/cmd_validate/process.py.
Sorry that I didn't tell you first. I guess we should remove the docbuild validate subcommand. Perhaps you should also move these functions and move it here.
| # --- LAZY FIX --- | ||
| # If the user is just asking for help, STOP HERE. | ||
| # Click will handle the help display for the subcommands automatically. | ||
| if ctx.resilient_parsing or "--help" in sys.argv or "-h" in sys.argv: |
Contributor
There was a problem hiding this comment.
Is it really needed to explicitly check for --help/-h? Shouldn't that be the task of Click? This looks like a code smell to me. 😉
Comment on lines
+11
to
+20
| def flatten_dict(d: dict[str, Any], prefix: str = "") -> dict[str, Any]: | ||
| """Flatten a nested dictionary into dotted keys (e.g., app.logging.level).""" | ||
| items: list[tuple[str, Any]] = [] | ||
| for k, v in d.items(): | ||
| new_key = f"{prefix}.{k}" if prefix else k | ||
| if isinstance(v, dict): | ||
| items.extend(flatten_dict(v, new_key).items()) | ||
| else: | ||
| items.append((new_key, v)) | ||
| return dict(items) |
Contributor
There was a problem hiding this comment.
This function looks quite general. Maybe move this function somewhere in src/docbuild/utils/? Use flatten.py?
Comment on lines
+11
to
+20
| def flatten_dict(d: dict[str, Any], prefix: str = "") -> dict[str, Any]: | ||
| """Flatten a nested dictionary into dotted keys (e.g., app.logging.level).""" | ||
| items: list[tuple[str, Any]] = [] | ||
| for k, v in d.items(): | ||
| new_key = f"{prefix}.{k}" if prefix else k | ||
| if isinstance(v, dict): | ||
| items.extend(flatten_dict(v, new_key).items()) | ||
| else: | ||
| items.append((new_key, v)) | ||
| return dict(items) |
Contributor
There was a problem hiding this comment.
Is converting the list to a dict really necessary? It should be left to the caller.
Additionally, maybe turn the function into a generator. This won't hold the entire list in memory, making it more efficient.
def flatten_dict(d: dict[str, Any], prefix: str = "") -> Generator[tuple[str, Any], None, None]:
"""
A generator that yields flattened key-value pairs from a nested dictionary.
:param prefix: The accumulated path of keys from higher levels of
nesting. This is used to build the dotted string (e.g., "parent.child").
Defaults to an empty string for the root level.
:yields: A tuple containing the (dotted_key, value) pair.
"""
for k, v in d.items():
# Construct the new key based on the current nesting level
new_key = f"{prefix}.{k}" if prefix else k
if isinstance(v, dict):
# yield from recursively calls the generator and yields its results
yield from flatten_dict(v, new_key)
else:
# Yield the final flattened key and its non-dict value
yield new_key, v| _print_section("Application Configuration", context.appconfig.model_dump(), "app", flat, "cyan") | ||
|
|
||
| if (env or show_all) and context.envconfig: | ||
| _print_section("Environment Configuration", context.envconfig.model_dump(), "env", flat, "yellow") |
Contributor
There was a problem hiding this comment.
I'm not sure why, but when I use my env.devel.toml file, I get a traceback:
Traceback of docbuild config list --env
$ docbuild --env-config=env.devel.toml config list --env
# Environment Configuration
Traceback (most recent call last):
File "/home/toms/repos/GH/opensuse/docbuild.git/main/.venv/bin/docbuild", line 10, in <module>
sys.exit(cli())
~~~^^
File "/home/toms/repos/GH/opensuse/docbuild.git/main/.venv/lib/python3.14/site-packages/click/core.py", line 1514, in __call__
return self.main(*args, **kwargs)
~~~~~~~~~^^^^^^^^^^^^^^^^^
File "/home/toms/repos/GH/opensuse/docbuild.git/main/.venv/lib/python3.14/site-packages/click/core.py", line 1435, in main
rv = self.invoke(ctx)
File "/home/toms/repos/GH/opensuse/docbuild.git/main/.venv/lib/python3.14/site-packages/click/core.py", line 1902, in invoke
return _process_result(sub_ctx.command.invoke(sub_ctx))
~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^
File "/home/toms/repos/GH/opensuse/docbuild.git/main/.venv/lib/python3.14/site-packages/click/core.py", line 1902, in invoke
return _process_result(sub_ctx.command.invoke(sub_ctx))
~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^
File "/home/toms/repos/GH/opensuse/docbuild.git/main/.venv/lib/python3.14/site-packages/click/core.py", line 1298, in invoke
return ctx.invoke(self.callback, **ctx.params)
~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/toms/repos/GH/opensuse/docbuild.git/main/.venv/lib/python3.14/site-packages/click/core.py", line 853, in invoke
return callback(*args, **kwargs)
File "/home/toms/repos/GH/opensuse/docbuild.git/main/.venv/lib/python3.14/site-packages/click/decorators.py", line 34, in new_func
return f(get_current_context(), *args, **kwargs)
File "/home/toms/repos/GH/opensuse/docbuild.git/main/src/docbuild/cli/cmd_config/list.py", line 59, in list_config
_print_section("Environment Configuration", context.envconfig.model_dump(), "env", flat, "yellow")
~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/toms/repos/GH/opensuse/docbuild.git/main/src/docbuild/cli/cmd_config/list.py", line 29, in _print_section
print_json(data=data)
~~~~~~~~~~^^^^^^^^^^^
File "/home/toms/repos/GH/opensuse/docbuild.git/main/.venv/lib/python3.14/site-packages/rich/__init__.py", line 106, in print_json
get_console().print_json(
~~~~~~~~~~~~~~~~~~~~~~~~^
json,
^^^^^
...<8 lines>...
sort_keys=sort_keys,
^^^^^^^^^^^^^^^^^^^^
)
^
File "/home/toms/repos/GH/opensuse/docbuild.git/main/.venv/lib/python3.14/site-packages/rich/console.py", line 1790, in print_json
json_renderable = JSON.from_data(
data,
...<7 lines>...
sort_keys=sort_keys,
)
File "/home/toms/repos/GH/opensuse/docbuild.git/main/.venv/lib/python3.14/site-packages/rich/json.py", line 85, in from_data
json = dumps(
data,
...<6 lines>...
sort_keys=sort_keys,
)
File "/home/toms/.local/share/uv/python/cpython-3.14.3-linux-x86_64-gnu/lib/python3.14/json/__init__.py", line 242, in dumps
**kw).encode(obj)
~~~~~~^^^^^
File "/home/toms/.local/share/uv/python/cpython-3.14.3-linux-x86_64-gnu/lib/python3.14/json/encoder.py", line 202, in encode
chunks = self.iterencode(o, _one_shot=True)
File "/home/toms/.local/share/uv/python/cpython-3.14.3-linux-x86_64-gnu/lib/python3.14/json/encoder.py", line 263, in iterencode
return _iterencode(o, 0)
File "/home/toms/.local/share/uv/python/cpython-3.14.3-linux-x86_64-gnu/lib/python3.14/json/encoder.py", line 182, in default
raise TypeError(f'Object of type {o.__class__.__name__} '
f'is not JSON serializable')
TypeError: Object of type IPv4Address is not JSON serializable
when serializing dict item 'host'
when serializing dict item 'server'
It looks like it's a problem with our models?
Comment on lines
+8
to
+10
| @click.command(name="validate") | ||
| @click.pass_context | ||
| def validate_config(ctx: click.Context) -> None: |
Contributor
There was a problem hiding this comment.
This will validate all config files. I think, that's unfortunate; maybe you only want to validate the XML? Or the env config?
I would introduce the options --portal, --env, and --app to restrict it to the specific config only. Additionally, the code below also introduces an --all option. The idea would be that --all and the other three options are mutually exclusive.
This how you could add more arguments with an opt-in logic:
Suggested change
| @click.command(name="validate") | |
| @click.pass_context | |
| def validate_config(ctx: click.Context) -> None: | |
| @click.command(name="validate") | |
| @click.option('portal_flag', | |
| '--portal/--no-portal', | |
| default=False, | |
| help="Validate the portal settings" | |
| ) | |
| @click.option('env_flag', | |
| '--env/--no-env', | |
| default=False, | |
| help="Validate the environment settings" | |
| ) | |
| @click.option('app_flag', | |
| '--app/--no-app', | |
| default=False, | |
| help="Validate the app settings" | |
| ) | |
| @click.option('all_flag', | |
| '--all', | |
| is_flag=True, | |
| default=True, | |
| help="Validate everything (Exclusive)." | |
| ) | |
| @click.pass_context | |
| def validate_config(ctx: click.Context, portal_flag, env_flag, app_flag, all_flag) -> None: |
With that options, you would have
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related Issue #112
Key Changes
Lazy-Loading & Help Logic (
src/docbuild/cli/cmd_cli.py)Implemented a "Help Shield": The root
clifunction now detects if a help flag (--helpor-h) is present insys.argvbefore attempting to load any configuration files.Task-Oriented Config Subcommands
Merged Resources: Replaced the separate config application and config environment commands with unified config list and config validate commands. Added config list: Supports
--app,--env, and--portalflags with an optional--flatoutput mode for "git-style" dotted configuration views. Added config validate: Centralizes validation logic for both application and environment files in one place.Test Suite Stabilization
Metadata Logging: Updated
tests/cli/cmd_metadata/test_cmd_metadata.pyto useunittest.mock.patchon the module-level logger. This ensures tests passing by bypassing caplog capture issues in async environments.Help Verification: Added
tests/cli/test_help.pyto specifically verify thatload_app_configis never called when the help flag is passed.